Skip to content

Allow Adding Sms / Email before Login#1355

Merged
fadi-george merged 4 commits intomainfrom
fadi/sdk-2916-web-sdk-email-prompt-doesnt-allow-an-email-subscription
Aug 25, 2025
Merged

Allow Adding Sms / Email before Login#1355
fadi-george merged 4 commits intomainfrom
fadi/sdk-2916-web-sdk-email-prompt-doesnt-allow-an-email-subscription

Conversation

@fadi-george
Copy link
Copy Markdown
Contributor

@fadi-george fadi-george commented Aug 20, 2025 •
edited
Loading

Description

1 Line Summary

  • Allow adding email before login

Details

  • On user initialize, well check for a onesignal. If it doesnt exists, we will create a local id.
  • For addSms or addEmail we will we need to enqueue a login operation if one isnt already in queue. If there are two login operations (either for addSms + addEmail or login + addEmail) then it will error.
  • Trackevent still needs a login operation before it otherwise the very first track event op for a new user (local onesignal id) will get stuck and never executed.
  • User tags (as how it was before) will get erased if user is logged in later.

Systems Affected

  • WebSDK
  • Backend
  • Dashboard

Validation

Tests

Info

Checklist

  • All the automated tests pass or I explained why that is not possible
  • I have personally tested this on my machine or explained why that is not possible
  • I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

  • Don't use default export
  • New interfaces are in model files

Functions:

  • Don't use default export
  • All function signatures have return types
  • Helpers should not access any data but rather be given the data to operate on.

Typescript:

  • No Typescript warnings
  • Avoid silencing null/undefined warnings with the exclamation point

Other:

  • Iteration: refrain from using elem of array syntax. Prefer forEach or use map
  • Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Info

Checklist

  • I have included screenshots/recordings of the intended results or explained why they are not needed

Related Tickets

This change is Reviewable

@fadi-george fadi-george requested review from jkasten2 and sherwinski and removed request for sherwinski August 20, 2025 23:47
@fadi-george fadi-george force-pushed the fadi/sdk-2916-web-sdk-email-prompt-doesnt-allow-an-email-subscription branch 3 times, most recently from f041aa1 to f761617 Compare August 21, 2025 17:57
@fadi-george fadi-george force-pushed the fadi/sdk-2916-web-sdk-email-prompt-doesnt-allow-an-email-subscription branch 6 times, most recently from ec4ae70 to 3a3fa3d Compare August 21, 2025 20:30
@fadi-george fadi-george force-pushed the fadi/sdk-2916-web-sdk-email-prompt-doesnt-allow-an-email-subscription branch from 3a3fa3d to a3323ed Compare August 21, 2025 20:45
fadi-george
fadi-george commented Aug 21, 2025
View reviewed changes
Comment thread src/core/executors/RefreshUserOperationExecutor.test.ts
});

test('should preserve cached push subscription when updating models', async () => {
setUpdateSubscriptionResponse({ subscriptionId: SUB_ID_2 });
Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting rid of extraneous calls

sherwinski
sherwinski reviewed Aug 22, 2025
View reviewed changes
Comment thread src/page/managers/LoginManager.ts
);
];

await Promise.all(promises);
Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double checking, do we not care about the order in which TransferSubscriptionOperation and LoginUserOperation are run?

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically it doesnt matter since the operations get grouped anyway.

Comment thread src/onesignal/User.ts
get onesignalId(): string | undefined {
const oneSignalId = OneSignal.coreDirector.getIdentityModel().onesignalId;
return !IDManager.isLocalId(oneSignalId) ? oneSignalId : undefined;
get onesignalId(): string {
Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only worry about this is ensuring that we have a IDManager.isLocalId(oneSignalId) check anywhere needed that invokes this getter

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a public api but its only used here.

sherwinski
sherwinski reviewed Aug 22, 2025
View reviewed changes
Comment thread src/onesignal/User.ts
IdentityConstants.ONESIGNAL_ID,
onesignalId,
ModelChangeTags.NO_PROPOGATE,
);
Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking and very nit but I just realized: shouldn't this be NO_PROPAGATE?

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Youre right, updated!

@fadi-george fadi-george merged commit bd198fe into main Aug 25, 2025
4 checks passed
@fadi-george fadi-george deleted the fadi/sdk-2916-web-sdk-email-prompt-doesnt-allow-an-email-subscription branch August 25, 2025 16:11
@fadi-george fadi-george mentioned this pull request Aug 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sherwinski sherwinski sherwinski approved these changes

@jkasten2 jkasten2 Awaiting requested review from jkasten2

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fadi-george @sherwinski